Skip to content

test(linter): add linter to coverage task#3591

Closed
DonIsaac wants to merge 2 commits intomainfrom
don/linter/test/conformance
Closed

test(linter): add linter to coverage task#3591
DonIsaac wants to merge 2 commits intomainfrom
don/linter/test/conformance

Conversation

@DonIsaac
Copy link
Contributor

@DonIsaac DonIsaac commented Jun 9, 2024

Makes cargo coverage run the linter. It can be run in isolation with cargo coverage linter.

The linter is run with fixes and all rules enabled (except nursery rules) in an identical fashion to oxlint -W all --fix. The fixed code is then re-parsed and checked for errors. Tests will fail if parsing fails or if semantic analysis produces more errors the second time around than when the original source code is analyzed.

I've already found several problems with fixes, including 2 panics. One of them was fixed in #3590, and the other is fixed in this PR.

@DonIsaac DonIsaac added the A-linter Area - Linter label Jun 9, 2024
@graphite-app
Copy link
Contributor

graphite-app bot commented Jun 9, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 9, 2024

CodSpeed Performance Report

Merging #3591 will not alter performance

Comparing don/linter/test/conformance (8cd8450) with main (f0b689d)

Summary

✅ 22 untouched benchmarks

@DonIsaac DonIsaac marked this pull request as ready for review June 9, 2024 03:19
@Boshen
Copy link
Member

Boshen commented Jun 9, 2024

Should we add this to https://github.com/oxc-project/oxlint-ecosystem-ci/actions/workflows/ecosystem-ci.yml instead 🤔 It's quite difficult to interpret the snapshot results.

This also makes the conformance suite even slower to compile :-(

@DonIsaac
Copy link
Contributor Author

DonIsaac commented Jun 9, 2024

It appears to add 50s to the conformance CI job. If we think this is too high I can move it to the ecosystem CI job, though it may not catch some of the weirder fixes (e.g. labels)

@Boshen
Copy link
Member

Boshen commented Jun 9, 2024

It appears to add 50s to the conformance CI job. If we think this is too high I can move it to the ecosystem CI job, though it may not catch some of the weirder fixes (e.g. labels)

Let's try and move this to ecosystem CI with fixes applied to running oxlint, and then check whether the fixed file is good, maybe by running oxlint over it again?

@Boshen Boshen marked this pull request as draft June 12, 2024 08:18
@Boshen
Copy link
Member

Boshen commented Jun 12, 2024

Moving to oxc-project/oxc-ecosystem-ci#11

@Boshen Boshen closed this Jun 12, 2024
@Boshen Boshen deleted the don/linter/test/conformance branch October 14, 2024 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-linter Area - Linter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants